-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only check Cookie paths when sending requests #322
Only check Cookie paths when sending requests #322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! I agree with the principle of only checking paths when sending and not when receiving. Please see inline comment for a possible change to the implementation.
I'm currently traveling to RubyKaigi, but I should be able to merge and test this next week.
Hey @jeremyevans – thank you for taking a look ... but I can't see the suggested change anywhere. 🤔 I know Github has been hiccuping a bit today, so maybe it just got lost?! Happy to make some changes – I just don't know what they are! Safe travels. |
@christhesoul Sorry about that, not sure why the inline comment didn't get submitted. I think it is best to not introduce new methods for this, and just move the path check from |
116ae60
to
0ce52b1
Compare
0ce52b1
to
02fec34
Compare
@jeremyevans Good idea! And the change seems much simpler as a result. I like it. Let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
My pleasure! |
Hey wonderful Rack::Test folks 👋
I come with little open source contribution experience, but plenty of gratitude for the folks that do and positive intent.
This PR aims to resolve a cookie issue where I believe the Rack::Test implementation differs from the cookie specification.
I've done some testing and checked the spec, in these cases it's possible to set a cookie with a path parameter different to the current path of the browser. The cookie will only be sent to the server when the browser makes a request to that path.
The above is taken from https://httpwg.org/specs/rfc6265.html#rfc.section.8.6
In the current implementation of Rack::Test the same
valid?
method is performed when setting and sending cookies. The validity rules should differ; the path validity can be ignored when setting cookies.This PR is an attempt to demonstrate the problem, double check on my interpretation of the spec, and propose a solution.
Thanks in advance for your time.